Seeder/secrets manager scenes - #8255
Conversation
Add four SM seeder scenes (project, secret, service account, access policy) that persist real rows via the commercial EF repositories, and wire AddSecretsManagerEfRepositories into SeederApi Startup so the scenes override the OSS Noop registrations. Scenes mirror OrganizationCollectionScene: inject repositories and IManglerService, load and validate the org, encrypt fields under the org key, and return the mangle map. Adds an end-to-end integration test exercising all four scenes through POST /seed and asserting real DB rows with round-tripped encryption.
The Secrets Manager seeder scenes claimed to target an SM-enabled org but never checked Organization.UseSecretsManager, so they could seed impossible fixtures (SM data on an SM-off org), and bad refs surfaced as raw FK 500s. Add a UseSecretsManager guard to OrganizationProjectScene, OrganizationSecretScene, OrganizationServiceAccountScene, and OrganizationAccessPolicyScene. The guard throws InvalidOperationException, which SceneExecutor/SeedController surface as a 400 with a clean message. Add an integration test seeding a non-SM Enterprise org (via OrganizationOverrides.UseSecretsManager=false) and asserting the project scene returns 400 and writes no row.
Extract the duplicated org-load + not-found + Secrets Manager-enabled guard into a GetSecretsManagerOrganizationOrThrowAsync extension on IOrganizationRepository, and call it from the four SM scenes. Keeps the exception messages byte-identical so the BadRequest integration test and HTTP 400 mapping are unaffected.
nthompson-bitwarden
added
the
t:misc
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8255 +/- ##
=======================================
Coverage 63.48% 63.48%
=======================================
Files 2430 2430
Lines 104928 104928
Branches 9505 9505
=======================================
+ Hits 66614 66615 +1
+ Misses 36034 36033 -1
Partials 2280 2280 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the four new Secrets Manager seeder scenes ( Two existing reviewer threads remain unresolved and are not restated here: the OSS build-guard question on the Code Review DetailsNo new findings at or above the reporting threshold. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
SeederApi only calls AddSecretsManagerEfRepositories(), which lives in Commercial.Infrastructure.EntityFramework. Nothing in util/SeederApi or test/SeederApi.IntegrationTest uses Bit.Commercial.Core, so the reference only pulled Commercial.Core (and CsvHelper transitively) into the build and container image. Regenerated both packages.lock.json files.
theMickster
left a comment
theMickster
left a comment
There was a problem hiding this comment.
I think we have refactoring work to do in the scenes and a couple things for thought/discussion. Thanks!
| mangleMap: manglerService.GetMangleMap()); | ||
| } | ||
|
|
||
| private static BaseAccessPolicy BuildPolicy(Grant grant) => |
There was a problem hiding this comment.
util/Seeder/Factories folder. Doing this keeps the scene's responsibility to only being the orchestrator and not the creator of seeds, and moving this allows for code reuse by the CLI (or future scenes).
| throw new InvalidOperationException($"Organization {request.OrganizationId} not found."); | ||
| } | ||
|
|
||
| var project = new Project |
There was a problem hiding this comment.
c5966eb#r3860957845
| throw new InvalidOperationException($"Organization {request.OrganizationId} not found."); | ||
| } | ||
|
|
||
| var secret = new Secret |
There was a problem hiding this comment.
c5966eb#r3860957845
| throw new InvalidOperationException($"Organization {request.OrganizationId} not found."); | ||
| } | ||
|
|
||
| var serviceAccount = new ServiceAccount |
There was a problem hiding this comment.
c5966eb#r3860957845
There was a problem hiding this comment.
🎨 I don't love that we have added an extensions helper class to the namespace that contains scenes. I also really don't want to have this in an base class an inheritance coupling 🤮
Where else is a better home for this (and future) extensions?
Secondarily, I am also not convinced at first glance that this must be public; wouldn't marking this as internal be more accurate/applicable? 🧐
| /// Creates a Secrets Manager secret (key/value/note encrypted with the organization's symmetric key) | ||
| /// for an existing Secrets Manager-enabled organization, optionally associating it with projects. | ||
| /// </summary> | ||
| public class OrganizationSecretScene( |
There was a problem hiding this comment.
❓Should we also consider injecting IProjectRepository and perhaps call await projectRepository.ProjectsAreInOrganization(request.ProjectIds.ToList(), organization.Id) to guard that the project is properly bound to the correct organization?
There was a problem hiding this comment.
❓Should we also consider injecting
IProjectRepositoryand perhaps callawait projectRepository.ProjectsAreInOrganization(request.ProjectIds.ToList(), organization.Id)to guard that the project is properly bound to the correct organization?
Thanks Mick, good call. Rather than inline the ProjectsAreInOrganization call, I pulled it into a shared ThrowIfProjectsNotInOrganizationAsync extension (SeederRepositoryGuardExtensions).
I think there's a broader pattern here that might be worth applying to other scenes 🤔
There was a problem hiding this comment.
♻️ A local multi-agent Claude Code review brought to light the following finding. It's verbose, but I think it's correct that we slow down to analyze if/how we properly introduce this project reference into the Seeder and thus into the SeederApi.IntegrationTest test library.
Don't take Claude's word for word on the work to do, but let's be wise and double-check the recommendations.
Commercial project reference and SM repo registration are not gated behind the OSS build guard
`util/SeederApi/SeederApi.csproj:15`
**Caught by:** Code quality agent
This PR adds <ProjectReference Include="..\..\bitwarden_license\src\Commercial.Infrastructure.EntityFramework\Commercial.Infrastructure.EntityFramework.csproj" /> unconditionally, and util/SeederApi/Startup.cs:39 calls services.AddSecretsManagerEfRepositories() unconditionally.
Both established sibling sites gate exactly this dependency:
src/Api/Api.csproj:31-38andsrc/Admin/Admin.csproj:21-28wrap the sameCommercial.Infrastructure.EntityFrameworkreference in<Choose><When Condition="!$(DefineConstants.Contains('OSS'))">.src/Api/Startup.cs:204-212andsrc/Admin/Startup.cs:98-103select betweenservices.AddOosServices()(Noop SM repositories) andservices.AddSecretsManagerEfRepositories()via#if OSS/#else.
Two consequences:
util/SeederApisits in the AGPL portion of the tree and now has a hard, ungated compile-time dependency onbitwarden_licensesource. An OSS-defined build of SeederApi cannot restore or compile.test/SeederApi.IntegrationTestbecomes the first project under./testto transitively referencebitwarden_license(grep -rn bitwarden_license test --include='*.csproj'currently returns nothing), which blurs thedotnet test ./test("OSS solution") vsdotnet test ./bitwarden_license/test("Bitwarden solution") split in.github/workflows/test.yml:52-56.
Suggested fix: mirror the sibling pattern — move the ProjectReference into a <Choose><When Condition="!$(DefineConstants.Contains('OSS'))"> block and wrap the AddSecretsManagerEfRepositories() call in #if OSS / #else, taking services.AddOosServices() on the OSS side.
Note the fix is not purely mechanical: AddOosServices() (src/SharedWeb/Utilities/ServiceCollectionExtensions.cs:386-394) registers Noops for IProjectRepository, ISecretRepository, IServiceAccountRepository and ISecretVersionRepository, but not IAccessPolicyRepository. Since SeederApi's default host builder validates the container on build and AddScenes() registers every scene's concrete type, OrganizationAccessPolicyScene would fail startup validation on the OSS side unless a Noop IAccessPolicyRepository is added too. That gap should be resolved as part of the gating rather than sidestepped by leaving the dependency ungated.
Move entity creation out of the Secrets Manager seeder scenes into new Factories classes so scenes only orchestrate. Adds ProjectSeeder, ServiceAccountSeeder, SecretSeeder, and AccessPolicySeeder (which owns the grantee/grantable enums and policy switch), enabling reuse by the CLI and future scenes.
Relocate OrganizationRepositoryExtensions to a dedicated Extensions folder and mark it internal, addressing PR review feedback that a helper class did not belong in the Scenes namespace and need not be public.
OrganizationSecretScene trusted caller-supplied ProjectIds on faith, so a stale or cross-org project ID silently seeded a broken secret-project link. Inject IProjectRepository and validate ownership via ProjectsAreInOrganization before creating the secret, mirroring production SM authorization and the existing org-lookup guard pattern.
OrganizationAccessPolicyScene only verified the org had Secrets Manager enabled; it never checked that a grant grantee/grantable belonged to the target org, so it would persist cross-org access policies. Validate both grantee and grantable across all valid combos before creating policies, mirroring the OrganizationSecretScene project guard. Consolidate the branch-new per-repository guard extensions into a single SeederRepositoryGuardExtensions class covering organization, project, service account, group, and organization user membership checks. Add negative integration tests for cross-org grantable and grantee ids and update the seeder regression doc.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/QA-2400
📔 Objective
Add Secrets Manager seeder scenes to the SeederApi so tests and local setups can persist real SM fixtures (projects, secrets, service accounts, access policies) against an SM-enabled org.
What changed:
OrganizationProjectScene,OrganizationSecretScene,OrganizationServiceAccountScene, andOrganizationAccessPolicyScene— that persist real rows via the commercial EF repositories. Each mirrorsOrganizationCollectionScene: inject repositories andIManglerService, load and validate the org, encrypt fields under the org key, and return the mangle map.AddSecretsManagerEfRepositoriesinto SeederApiStartupso the scenes override the OSS Noop registrations.Organization.UseSecretsManager, so they could seed impossible fixtures (SM data on an SM-off org) and bad refs surfaced as raw FK 500s. The guard throwsInvalidOperationException, whichSceneExecutor/SeedControllersurface as a clean 400.GetSecretsManagerOrganizationOrThrowAsyncextension onIOrganizationRepository, called by all four scenes. Exception messages are kept byte-identical so the BadRequest integration test and HTTP 400 mapping are unaffected.Tests:
POST /seed, asserting real DB rows with round-tripped encryption.OrganizationOverrides.UseSecretsManager=false) asserting the project scene returns 400 and writes no row.